Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add IPv6 support #34

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Feature: Add IPv6 support #34

wants to merge 8 commits into from

Conversation

adjivas
Copy link

@adjivas adjivas commented Feb 14, 2025

Hello,

This PR adds the RegisterIP/BindingIP fields, we can set them with IPv4 or IPv6 values
For not introduce any breaking changes, the RegisterIPv4/BindingIPv4 are the fall back of the RegisterIP/BindingIP fields.
It's adds a validation rule to avoid missing or duplicate field.

I test this PR with this fix of the Free5gs's Nrf to have this valid results:
image
image

@adjivas adjivas changed the title IPv6 support Feature: Add IPv6 support Feb 19, 2025
@andy89923 andy89923 self-requested a review February 20, 2025 07:02
Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Sorry, I am not digging into the implementation yet.
Here are some quick suggestions for this PR!

Welcome to discuss, thanks!

Comment on lines 111 to 114
RegisterIPv4 string `yaml:"registerIPv4,omitempty" valid:"host,optional"` // IP that is registered at NRF.
RegisterIP string `yaml:"registerIP,omitempty" valid:"host,optional"` // IP that is registered at NRF.
BindingIPv4 string `yaml:"bindingIPv4,omitempty" valid:"host,optional"` // IP used to run the server in the node.
BindingIP string `yaml:"bindingIP,omitempty" valid:"host,optional"` // IP used to run the server in the node.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need two variables that stand for the same thing?
Or using RegisterIP and BindingIP only?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegisterIP/BindingIP both works with IPv6 and Ipv4
But, if we remove RegisterIPv4/BindingIPv4, we will have to adapt every configuration files that use it
This is why the solution of this MR is to keep RegisterIPv4/BindingIPv4 fields, to use them as a the fallback of RegisterIP/BindingIP

ueauService := (*udmInstance.NfServices)[0]
ueauEndPoint := (*ueauService.IpEndPoints)[0]
port := uint16(ueauEndPoint.Port)
if len(udmInstance.Ipv6Addresses) > 0 && udmInstance.NfServices != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UDM should register with IPv6 for this code to work.
I think this change can be done in another PR or wait for UDM to have a PR ready.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That a work in progress, I begin to work on the IPv6 support for UDM adjivas/udm#1
I will open this PR to the Free5gc's UDM later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants